Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🪟 🎉 Add datepicker for date/date-time fields in connector form #19678

Merged
merged 42 commits into from
Dec 7, 2022

Conversation

josephkmh
Copy link
Contributor

@josephkmh josephkmh commented Nov 21, 2022

What

  • Adds a datepicker component
  • Displays this component in the connector form when a field with format date or date-time is rendered
datepicker.mov

How

  • Adds react-datepicker and styles it with our design tokens
  • Adds a new <Datepicker /> component which renders a text input with a datepicker button.
  • Only UI widgets with the correct json-schema format of date or date-time will see the datepicker. Many connectors today have date fields that are typed as string. In these cases a normal text input will be rendered (see "End date" in the video above).

Recommended reading order

Top to bottom

🚨 User Impact 🚨

The datepicker is currently behind the connector.form.useDatepicker experiment flag. It's enabled by default on OSS, and can be toggled easily on LaunchDarkly for cloud.

The plan is to roll out the datepicker to all users on cloud as well, and remove the flag if all goes well. Even if there is a bug with the datepicker itself, a normal <Input /> is rendered where the user can enter the UTC timestamp manually.

@octavia-squidington-iv octavia-squidington-iv added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Nov 21, 2022
@josephkmh josephkmh changed the title 🪟 🎉 Datepicker for timestamp fields 🪟 🎉 Datepicker for date fields Nov 21, 2022
@josephkmh josephkmh changed the title 🪟 🎉 Datepicker for date fields 🪟 🎉 Add datepicker for date/date-time fields in connector form Nov 24, 2022
@josephkmh josephkmh marked this pull request as ready for review November 25, 2022 10:28
Copy link
Contributor

@lmossman lmossman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple small comments and one visual bug I found when testing

@flash1293
Copy link
Contributor

It seems like error states for missing value and malformed input are not shown:
Screenshot 2022-12-05 at 16 55 25
Screenshot 2022-12-05 at 16 55 29

@flash1293
Copy link
Contributor

I understand that it would be a difficult task to show the time in the users local time zone, however what about setting the timeCaption property on the picker to "Time (UTC)" to emphasize this? Otherwise I'm concerned this way of setting the time makes it very easy for users to miss they need to convert to UTC before selecting the time:
Screenshot 2022-12-05 at 16 58 10
Screenshot 2022-12-05 at 17 03 20

@josephkmh
Copy link
Contributor Author

It seems like error states for missing value and malformed input are not shown: Screenshot 2022-12-05 at 16 55 25 Screenshot 2022-12-05 at 16 55 29

@flash1293 looks like you're using the Bing Ads source connector, right? That connector spec unfortunately doesn't specify a pattern for the start_date:

"reports_start_date": {
  "type": "string",
  "order": 5,
  "title": "Reports replication start date",
  "format": "date",
  "default": "2020-01-01",
  "description": "The start date from which to begin replicating report data. Any data generated before this date will not be replicated in reports. This is a UTC date in YYYY-MM-DD format."
}

Which is what we depend on to do regex validation here when generating the yup schema. So any old string can be passed in 😕 Unfortunately quite a few connector specs are missing pattern or format: date | date-time which means the field won't be validated or the datepicker won't be shown respectively.

@flash1293
Copy link
Contributor

Should we auto-add this pattern if it doesn't exist or will it break things? If it's too dangerous to do it, we should probably extend the SAT to cover this situation (out of scope for this PR I'd guess)

@josephkmh
Copy link
Contributor Author

Should we auto-add this pattern if it doesn't exist or will it break things? If it's too dangerous to do it, we should probably extend the SAT to cover this situation (out of scope for this PR I'd guess)

I'm not sure I have enough context on the date types (there are five) to know if this would break anything - probably not? But my feeling is that keeping the connector definition specification as the source of truth for validation is the best approach. I'd rather advocate for consistency in the connector specs than patching missing fields on the frontend.

@lmossman
Copy link
Contributor

lmossman commented Dec 5, 2022

+1 to the timeCaption suggestion

Yeah I think the problem with trying to enforce the regex pattern in the frontend if it is missing on the connector spec is that different connectors may require different timestamp formats, so we can't implement a blanket logic to cover all connectors correctly. Though @sherifnada or @girarda correct me if I'm wrong here - I assume some timestamp formats are API-specific, but if it actually can be made consistent across all APIs that would be great

@flash1293
Copy link
Contributor

if it actually can be made consistent across all APIs that would be great

+1 for that, I think going with the current logic for now and extending the SAT is a good approach to get to a clean state without breaking things in the meantime.

@josephkmh
Copy link
Contributor Author

@flash1293 thanks for the idea about adding Time (UTC)! Just pushed that here.

Copy link
Contributor

@lmossman lmossman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM, tested locally once more and it seems to be working as expected. Nice work!

@sherifnada
Copy link
Contributor

if it actually can be made consistent across all APIs that would be great

agreed. But this will take a little bit longer than we want to merge this PR :) so I think we shouldn't assume the regex on the frontend

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend Related to the Airbyte webapp area/platform issues related to the platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants